Harden attestation CBOR parsing#36
Open
leanthebean wants to merge 1 commit into
Open
Conversation
Reject duplicate recognized top-level attestation keys in NitroValidator instead of using last-write-wins semantics for pcrs, certificate, cabundle, and other parsed fields. Require the COSE payload byte string and outer payload map to be fully consumed so signed trailing bytes cannot be silently ignored. Keep unknown keys skippable for AWS forward compatibility. Add regression coverage for duplicate parsed fields, unknown duplicate compatibility, and trailing signed bytes.
104884b to
9a573e9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Security issue
NitroValidator._parseAttestationaccepted duplicate top-level CBOR map keys with last-write-wins behavior. If a signed attestation payload contained twopcrsentries, the second one replaced the first in the returnedPtrs, allowing parsed PCR values to diverge from the first measurements in the signed document. The same overwrite behavior applied to other security-critical pointers such ascertificateandcabundle.The stock AWS-rooted deployment is protected from external attackers by the COSE P-384 signature check and the hard-pinned AWS Nitro root, and AWS NSM emits canonical attestation CBOR. This patch still removes the parser ambiguity so validation correctness does not depend on signer encoder behavior or a custom
ICertManagerdeployment never signing duplicate-key payloads.The parser also now rejects signed-but-unparsed trailing bytes: data after the declared entries in a definite-length payload map, data after an indefinite map break marker, and bytes after the COSE payload byte string all revert.
Fix
The parser now tracks a bitmask of recognized top-level keys. Any duplicate recognized field reverts with
duplicate attestation key; unknown keys remain skippable for forward compatibility.After parsing,
_parseAttestationrequires the map cursor to end exactly at the payload boundary, and it requires the payload byte string itself to end exactly atattestationTbs.length. Indefinite maps consume the break marker before the final boundary check.Tests
forge fmtforge testAdded regression coverage for:
pcrs,certificate, andcabundle;Security finding: CBOR duplicate-key last-write-wins and signed-but-unparsed trailing bytes in
src/NitroValidator.sol.